Skip to content

gh-118965: Share NotImplemented in subinterpreters #118966

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

gh-118965: Share NotImplemented in subinterpreters #118966

wants to merge 5 commits into from

Conversation

@sobolevn
Copy link
Member

@sobolevn sobolevn commented May 12, 2024 •
edited
Loading

I still use Py_NewRef despite the fact that NotImplemented is immortal:

static void
notimplemented_dealloc(PyObject *notimplemented)
{
    /* This should never get called, but we also don't want to SEGV if
     * we accidentally decref NotImplemented out of existence. Instead,
     * since Notimplemented is an immortal object, re-set the reference count.
     */
    _Py_SetImmortal(notimplemented);
}

@bedevere-app bedevere-app bot mentioned this pull request May 12, 2024
Open
@nineteendo
Copy link
Contributor

nineteendo commented May 12, 2024 •
edited
Loading

cpython/Lib/test/test_interpreters/test_api.py

Lines 1128 to 1130 in 176b42a

not_shareables = [
# singletons
NotImplemented,

Should be moved to

cpython/Lib/test/test_interpreters/test_api.py

Lines 1099 to 1101 in 176b42a

shareables = [
# singletons
None,

By the way, is there a reason ... is not shareable?

@sobolevn
Copy link
Member Author

@nineteendo thanks.

By the way, is there a reason ... is not shareable?

I don't think so. I plan to address it in the next PR.

@ericsnowcurrently
Copy link
Member

FWIW, there's no urgency here. The other singletons have a clear value in being shareable. However, NotImplemented and Ellipsis do not. There's nothing wrong with making them shareable. However, there isn't much clear advantage either, AFAIK.

erlend-aasland
erlend-aasland reviewed May 18, 2024
View reviewed changes
Python/crossinterp_data_lookup.h
Comment on lines +582 to +587
// NotImplemented
if (_xidregistry_add_type(xidregistry,
(PyTypeObject *)PyObject_Type(Py_NotImplemented),
_notimplemented_shared) != 0) {
Py_FatalError("could not register NotImplemented for cross-interpreter sharing");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we generalize this with a macro? (Future refactoring)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. Last I heard, Eric's plan was to eventually get a dedicated type slot for crossinterpreter registry things, so this will (hopefully) be obsolete sooner than later.

ZeroIntensity
ZeroIntensity reviewed Feb 9, 2025
View reviewed changes
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to work through the subinterpreter backlog a bit :)

Python/crossinterp_data_lookup.h
Comment on lines +582 to +587
// NotImplemented
if (_xidregistry_add_type(xidregistry,
(PyTypeObject *)PyObject_Type(Py_NotImplemented),
_notimplemented_shared) != 0) {
Py_FatalError("could not register NotImplemented for cross-interpreter sharing");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. Last I heard, Eric's plan was to eventually get a dedicated type slot for crossinterpreter registry things, so this will (hopefully) be obsolete sooner than later.

@ZeroIntensity
Copy link
Member

ZeroIntensity commented Feb 9, 2025 •
edited
Loading

Merging from main seemed to blow up CI. I think something changed with XI registries after this was created.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erlend-aasland erlend-aasland erlend-aasland left review comments

@ZeroIntensity ZeroIntensity ZeroIntensity left review comments

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

Assignees

No one assigned

Labels

awaiting core review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@sobolevn @nineteendo @ericsnowcurrently @ZeroIntensity @erlend-aasland